Add support for ListView #92
Conversation
grod220
force-pushed
the
podslice-remove-methods
branch
3 times, most recently
from
4f972f1 to
817eb9f
Compare
There was a problem hiding this comment.
Mostly small points, looks good to me!
As another naming idea, what we're really providing is a View on existing data, so it's like a Vec that won't reallocate, which also similar to an arena allocator.
So we could go with ListView or NoAllocVec or ArenaList. I kind of like the last one, since it might describe the situation best.
I have a slight preference for |
Agreed, I personally have never heard the term "arena" in this context before, and I assume most developers will whiff on the underlying meaning at first. However, "view" implies readonly, and that's not the case here. Here are a few ideas:
|
Agree with your point on the "View" implying read-only, but while the type has a "dynamic" size its capacity is fixed. In that sense it is a view. 😊 The only concern that I have with using |
grod220
force-pushed
the
podslice-remove-methods
branch
from
817eb9f to
56057f1
Compare
| let remainder = length_size | ||
| .checked_rem(data_align) | ||
| .ok_or(ProgramError::ArithmeticOverflow)?; | ||
| if remainder == 0 { | ||
| Ok(0) | ||
| } else { | ||
| data_align | ||
| .checked_sub(remainder) | ||
| .ok_or(ProgramError::ArithmeticOverflow) | ||
| } |
There was a problem hiding this comment.
This block could be simplified:
| let remainder = length_size | |
| .checked_rem(data_align) | |
| .ok_or(ProgramError::ArithmeticOverflow)?; | |
| if remainder == 0 { | |
| Ok(0) | |
| } else { | |
| data_align | |
| .checked_sub(remainder) | |
| .ok_or(ProgramError::ArithmeticOverflow) | |
| } | |
| length_size | |
| .checked_rem(data_align) | |
| .and_then(|rem| data_align.checked_sub(rem)) | |
| .ok_or(ProgramError::ArithmeticOverflow) |
There was a problem hiding this comment.
Unfortunately this triggers test failures. It fails when the data is already aligned because it doesn't handle the remainder == 0 case. For example, to align a 4-byte header for 2-byte data, the padding should be 0. This snippet calculates 2 - (4 % 2) which is 2 - 0 = 2.
There was a problem hiding this comment.
A few things I am realizing.
1 - With unstake program’s new settlement design, we no longer need the remove functions.
2 - Users of ListView will not be able to use PodSlice for read-only use (if they rely upon the default len bytes). Pod64 vs Pod32 defaults. Given ListView now has iterable methods, it sort of seems like it could replace both PodSlice & PodSliceMut.
Up for discussion if this is worth keeping or not. The benefits are better alignment handling, remove functions, and test coverage. But perhaps these features are not pressing any longer.
| let remainder = length_size | ||
| .checked_rem(data_align) | ||
| .ok_or(ProgramError::ArithmeticOverflow)?; | ||
| if remainder == 0 { | ||
| Ok(0) | ||
| } else { | ||
| data_align | ||
| .checked_sub(remainder) | ||
| .ok_or(ProgramError::ArithmeticOverflow) | ||
| } |
There was a problem hiding this comment.
Unfortunately this triggers test failures. It fails when the data is already aligned because it doesn't handle the remainder == 0 case. For example, to align a 4-byte header for 2-byte data, the padding should be 0. This snippet calculates 2 - (4 % 2) which is 2 - 0 = 2.
Currently,
PodSliceMuthas served as the data structure for dynamically sized list. As we expand functionality to this, it is starting to feel misnamed (as Slices generally aren't things that are resized).Changes made
ListViewstruct that serves as the future home for mutations involving ordered listsPodSliceMut: unpack, init, pushPodSliceMutas deprecated